fix(desktop): surface Tailscale Serve failures instead of failing silently - #4919
fix(desktop): surface Tailscale Serve failures instead of failing silently#4919GenKerensky wants to merge 1 commit into
Conversation
…ently `tailscale serve` was configured by the child backend on boot. When the tailnet refused — Serve not enabled, no HTTPS certs, CLI missing — the failure landed in a log line the user never sees: the app restarted and the toggle simply stayed off, with nothing explaining why. Enabling now preflights `tailscale serve` in the main process *before* persisting, so it can fail loudly with the CLI's own guidance and the `login.tailscale.com/f/serve` admin link. The failure surfaces three ways: a toast, an inline alert inside the setup dialog (which stays open so the user can retry), and a status line on the row. The URL is shown in full and selectable, with an "Open setup" button as a shortcut. Related fixes in the same path: - Disable tears Serve down from the main process. It previously relied on the child server's `acquireRelease` finalizer, which only exists if *that* child booted with Serve enabled — so after a failed relaunch, toggling off persisted `enabled: false` while `tailscale serve --https=<port>` stayed live on the tailnet. A teardown that fails now refuses to record itself as done and says the backend may still be reachable. `serve off` against an already-unbound port is treated as success, verified against the real CLI. - The switch reflects the persisted setting, not endpoint reachability. `status` came from an HTTP probe of the MagicDNS URL, so a provisioning cert or a brief tailnet blip rendered the switch off while Serve was live — and since disabling sits behind the on state, there was then no way to turn it off. The same reasoning applies to the Authorized clients section. - The toggle renders without LAN network access. Serve only proxies to loopback, so it never depended on network-accessible mode. MagicDNS is still resolved only on explicit opt-in, preserving the pingdotgg#2745 macOS TCC gate. - Failed CLI spawns report "Could not run the tailscale CLI. Is Tailscale installed and on PATH?" instead of a generic port message. - A persistence failure no longer tears down a pre-existing working binding, rollback covers interrupt and defect rather than only typed failure, and the MagicDNS cache is invalidated on user-initiated resolve so "start Tailscale and try again" is not defeated by a stale 60s miss. The configure URL is rebuilt from a literal origin and path with only an allowlisted `node` parameter, so no text lifted from stderr can reach the logged error message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014UZ1f7q6ZVbn6uqWgWRzh9
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // skipping the rollback on those exits is exactly the silent-exposure | ||
| // case this whole path exists to prevent. Best-effort, since the | ||
| // original persistence failure is what the caller needs to see. | ||
| Effect.onExit((exit) => { |
There was a problem hiding this comment.
🟠 High backend/DesktopServerExposure.ts:717
Both rollback paths in setTailscaleServeEnabled swallow their error with Effect.ignore, so after a successful tailnet change and a failed settings write, the method returns only DesktopTailscaleServePersistenceError while the tailnet is left in the wrong state. On the enable side, if disableTailscaleServe rollback fails, the new HTTPS binding stays live on the tailnet but settings/UI show Serve disabled — the exact silent exposure this code exists to prevent. On the disable side, if ensureTailscaleServe rollback fails, the HTTPS endpoint is gone but settings and stateRef still say enabled, leaving runtime state inconsistent with persisted/UI state. In both cases the caller cannot detect the rollback failure. Consider logging or attaching a warning so the exposure/inconsistency is surfaced rather than silently dropped.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/backend/DesktopServerExposure.ts around line 717:
Both rollback paths in `setTailscaleServeEnabled` swallow their error with `Effect.ignore`, so after a successful tailnet change and a failed settings write, the method returns only `DesktopTailscaleServePersistenceError` while the tailnet is left in the wrong state. On the enable side, if `disableTailscaleServe` rollback fails, the new HTTPS binding stays live on the tailnet but settings/UI show Serve disabled — the exact silent exposure this code exists to prevent. On the disable side, if `ensureTailscaleServe` rollback fails, the HTTPS endpoint is gone but settings and `stateRef` still say enabled, leaving runtime state inconsistent with persisted/UI state. In both cases the caller cannot detect the rollback failure. Consider logging or attaching a warning so the exposure/inconsistency is surfaced rather than silently dropped.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces significant new error handling, rollback logic, and IPC infrastructure for Tailscale Serve configuration. An unresolved review comment identifies that rollback failures are silently ignored, potentially leaving HTTPS exposure in an inconsistent state with settings - warranting human review. You can customize Macroscope's approvability policy. Learn more. |
Closes #2830.
What Changed
Enabling Tailscale HTTPS from desktop settings now reports failure instead of silently doing nothing.
tailscale servewas configured by the child backend on boot. When the tailnet refused it, the failure landed in a log line the user never sees — the app restarted and the toggle simply stayed off. Enabling now preflightstailscale servein the main process before persisting, so it can fail with the CLI's own guidance and thelogin.tailscale.com/f/serveadmin link. The failure surfaces three ways: a toast, an inline alert inside the setup dialog (which stays open so you can retry), and a status line on the Tailscale HTTPS row. The setup URL is shown in full and is selectable, with an Open setup button as a shortcut.Related fixes in the same path:
acquireReleasefinalizer, which only exists if that child booted with Serve enabled. After a failed relaunch it doesn't, so toggling off persistedenabled: falsewhiletailscale serve --https=<port>stayed live on the tailnet. A teardown that fails now refuses to record itself as done, and says the backend may still be reachable.serve offagainst an already-unbound port counts as success. The real CLI exits non-zero with "handler does not exist"; the tailnet is already in the state the user asked for, and treating it as failure would strand the toggle on and warn about an exposure that does not exist — biting hardest in exactly the case the eager teardown exists for.statuscame from an HTTP probe of the MagicDNS URL, so a provisioning cert or a brief tailnet blip rendered the switch off while Serve was live — and since disabling sits behind the on state, there was then no way to turn it off. The same reasoning applies to the Authorized clients section, which was gated on the same probe.Why
The bug this fixes is silence, not the underlying tailnet refusal. A user with Serve disabled on their tailnet flips the toggle, the app restarts, and the toggle is off again with no message — nothing distinguishes "your tailnet blocks this" from "the app is broken." #2830 describes exactly that: the screen hangs or the app restarts "with seemingly no effect," and "no visible errors were seen. (This is the key issue)."
Two design notes for review:
tailscaleprints auth keys and node names to stderr, and these errors are logged, so failures are classified into a closed set of labels and the prose lives at the UI edge. The one exception is the admin URL, and it is rebuilt from a literal origin and path keeping only an allowlistednodeparameter — validating the parsed host and path alone would let?authkey=tskey-…ride through into a logged message.UI Changes
Affects the Tailscale HTTPS row in Settings → Connections:
Note
Screenshots are being attached separately — I can't upload images through the API. The behaviour above was verified against a live tailnet; see Testing.
Testing
Unit tests cover the new behaviour: eager teardown asserts the exact
serve --https=<port> offinvocation, a failed teardown keepsenabled: trueand carries the exposure warning, the missing-binary case produces the PATH hint,no-existing-handleris tolerated, and the configure URL drops non-allowlisted query parameters.Verified end-to-end by driving the built desktop app against a live tailnet:
serve) → dialog stays open with the inline alert, full URL, and Open setup; error anchored to the Tailscale HTTPS row rather than the LAN row.https://<magicdns> → proxy http://127.0.0.1:<port>intailscale serve status, the endpoint returned HTTP 200, and the app came back with the toggle on; disable returnedNo serve configwith the endpoint unreachable and the toggle off.openExternalconfirmed to resolvetrue/falserather than reject, which is why the previous.catch()fallback was dead code.Checklist
Note
High Risk
Changes ordering of Tailscale Serve vs persisted settings and main-process network exposure teardown/rollback; mistakes could leave the tailnet exposed or block legitimate disable. Touches auth-adjacent exposure and sanitized CLI error boundaries.
Overview
Fixes silent Tailscale HTTPS toggles (#2830) by preflighting
tailscale servein the desktop main process before persisting, so enable/disable can fail with actionable errors (CLI diagnostics, sanitizedlogin.tailscale.com/f/servelink) instead of restarting with the switch still off.Exposure lifecycle: disable now runs
serve offin the main process (not only the child finalizer); failed teardown does not persistenabled: false;no-existing-handleronserve offis treated as success; rollback on failed settings writes (including interrupt/defect viaonExit) reverses preflight enable or re-binds after a failed disable.UI / IPC: new
resolveTailscaleHttpsEndpoint(on-demandtailscale status, cache invalidated on user action); Connections Tailscale HTTPS switch follows persisted settings, works without LAN network access, shows spinner/errors inline and in dialogs;isShareableOriginstops offering pairing URLs for custom schemes liket3code-dev://.@t3tools/tailscale: classified stderr labels (serve-not-enabled,no-https-certs), safe configure URL extraction, and shared user-facing CLI messages.Reviewed by Cursor Bugbot for commit 3317caa. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Surface Tailscale Serve failures with actionable errors and rollback in desktop server exposure
setTailscaleServeEnablednow preflightstailscale servebefore persisting settings, maps CLI failures (exit errors, spawn errors) to a structuredDesktopTailscaleServeConfigureErrorwith a user-safe message and optional adminconfigureUrl.resolveTailscaleHttpsEndpointmethod and IPC channel (desktop:resolve-tailscale-https-endpoint) allows the renderer to request a fresh MagicDNS resolution on demand.tailscaleServeEnabledflag.serve-not-enabled,no-https-certs) are classified in@t3tools/tailscale, andTailscaleCommandExitErrornow carries a sanitizedconfigureUrlwhen present in CLI output.requiresRelaunch=true, even when settings are unchanged, so the child process re-binds Serve.📊 Macroscope summarized 3317caa. 11 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.